-
-
Notifications
You must be signed in to change notification settings - Fork 114
Handle priority inversion for promoted bundled libraries #123
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Fixes arduino/Arduino#4064 Signed-off-by: Martino Facchin <[email protected]>
Adding binaries for all platforms, please replace the |
To me, the real solution is that bundled libraries, when updated, should stay internal rather than being placed in the user's sketchbook. (Not necessarily in exactly the same folder as the original bundled library, but at an equivalent place in the priority stack.) Placing internal libraries in the user's sketchbook has issues besides just changing their place in the priority order. Which isn't to say that I necessarily object to this particular patch, just that I don't think it solves the underlying problem. |
facchinm,
Meaning that a core library only trumps a user sketchbook library if the user sketchbook library does not support the core. My overall comment is that when dealing with complex decision trees and implementation details like this, it is very important to document the intended behaviors before implementing it. In this case, it appears that we are dealing with two sets of library selections/priorities.
So I would be against merging in this patch until the full behavior is documented and discussed, especially given that as dave mentioned above that it does not appear to solve the underlying problem. While I agree with dave's comment above about it not solving the underlying problem (I went into detail on this in the other issue), I do not believe it is as simple as just creating a separate IDE bundled library area either. For example, I have many different IDEs installed so I can test with each of them. For example, it isn't just about keeping the bundled libraries separate, but also keeping each IDEs bundled libraries separate from each other. A lot of what makes this complicated, is that the library manager is looking at libraries as one big flat name space and we are actually dealing with multiple name spaces as well as library collisions. My view is that there are separate and distinct entities that each can have their own libraries:
The priority of selection is well defined. (user sketchbook, core, bundled) The questions becomes how do you do handle updates? I would argue that the 3 need to be kept separate and there should be a way to independently update each. One way to handle this kind of stuff and fully resolve all the issues would be to have the library .properties be able to indicate where the library repo is so that each library at each level can use the library manager to do its own library updates separately and independently from the others. In other words, the library manager can initially point to "known" libraries in a single name space, A core library can update itself and not affect the bundled library for a given IDE. A user can install a library manually in his sketchbook and override core and bundled and even get updates for his manually installed library and not affect the core or bundled libraries. The core library updates remain in the core area, the bundled updates remain in that particular IDE library area. Yes the library manager would work a bit differently in that it would get the repos from the .properties but it is a big win in that this keeps everything simple, clean and separate. |
David's proposal would be much better. As the maker of a 3rd party core with several customized copies of the bundled libraries, of course I like a solution which solves the problem where incompatible copies installed from the library manager break my core's well tested libraries. This has been quite a problem.
Imagine you're using Teensy with a WIZ820io ethernet module stacked on top. It works great, because Teensy's core provides its own Ethernet library, which has code specifically to support that other ethernet shield which isn't supported by the normal Arduino bundled library. Then one day, Arduino updates the bundled library with a couple changes, like a few lines to support their newest product: arduino-libraries/Ethernet@69e2575 and a minor change in the metadata wording: arduino-libraries/Ethernet@9a68147 The library managers display the popup which alters you to update to the latest version of the Ethernet library. So you do what most people do and click ok and complete the update. Suddenly your project no works. Now Arduino's updated Ethernet library is now being used. The really bad part is it compiles and runs without any error, but simply does not communicate because it lacks the code to support the hardware you're using. |
Thanks very much facchinm for your work towards a solution to this issue! I did a preliminary test of this using Arduino IDE 1.6.8 2016/03/04 03:33 with https://github.com/JChristensen/mighty-1284p/tree/v1.6.3 and https://github.com/MCUdude/MightyCore and the Arduino bundled versions of the libraries that are included with those cores installed to sketchbook. I am happy to test other hardware packages but I think this already demonstrate an issue that will require a change so I will wait to go further. Issues:
I think issue 2 could be solved by giving the core library higher include priority over the sketchbook library when they both have the same architecture value but that would prevent sketchbook libraries being able to purposefully override core libraries. I also would prefer the approach of installing Library Manager updates of Arduino bundled libraries(any library that is found at {Arduino IDE install folder}/libraries) to the Arduino15/libraries folder and adjusting the include priorities to place that location above the Arduino IDE install folder and below all other include locations, however I am very glad to see that the Arduino developers consider this issue worth fixing and will be glad of any solution that is found. Responses to @bperrybap's comments:
|
@PaulStoffregen It sounds like you have not understood my proposal. What you are describing is a re-ordering of the priority to The Teensy example you described is EXACTLY the problem I brought up and described above as well as went into detail in the thread in the other issue. The real issue is how updates are handled and where library updates are stored NOT how libraries are prioritized. The current priority is: It needs to be smarter as to how it handles its updates. There is simply no way to solve this by re-prioritization. For example, in Paul's Teensy example above, it is desired that a core library trump the IDE bundled library, which it will before doing an update with the existing library manager. The only way to solve this is to make the library manager smarter so that it can update libraries within each layer/entity separately. And by smarter I mean that the library manager treats updates for each layer/entity library separately and the builder will also know where to find these updates. In my previous post I didn't suggest where to put any of the updates (like @per1234 did in Arduino15/libraries) as I believe that is an implementation detail and before details like that should be considered, the overall concepts need to be discussed and agreed upon. Concept:What I'm suggesting is that each layer/entity (user sketchbook, core, bundled) gets its own update area. So lets look back an Paul's teensy example and see how that works when you have separate update areas and a rigid fixed priority of: user sketchbook, core, IDE bundled. An update to the IDE bundled ethernet library becomes available. This works because the library priority is fixed and each library within each layer can be updated without affecting any other layer's/entity's libraries. The concept is that there is not just one library. Each layer/entity can have its own version of a library with its own update repository that can be distinctly different and updated separately even through it has the same name as a library in another layer. This ensures that f a lower layer's library is updated it will not affect a high priority library or it's use. Detailed Examples:In terms of some additional details. Having separately updateable libraries in each layer would potentially require that there be more library locations. Existing locations and priority.
If the updates were done "in place" then the problems are solved, things are really simple, and no new library locations would be needed. Everything is kept separate and updates only affect the library being updated. If "in place" updates is not going to be allowed, then additional library locations would be needed. If you add other core and IDE library areas for updates, then the new library locations would be:
The priority is FIXED and NEVER changes. NOTE: the example below is when in place library updates is not being allowed. So now back to Paul’s example, And, if paul had an update to his teensy core ethernet library he could publish that to the teensy core ethernet library repo and the library manager would prompt that there is a Teensy core update for the ethernet library. By keeping the layers separate and allowing updates to each separately, you never have to muck with or deal with any priority issues. The priority is fixed and the library manager allows each layer to do its own updates. Cores can update individual core libraries without having to update its entire core and without disturbing any IDE or user libraries. Users can install their their own library and, if desired, override core or bundled libraries. The overall concept is to treat each library update separately rather than try to treat updates to all libraries with the same name as equivalent with a single repository. And to do that, you have support having a separate repo for each library in each layer and you have to either update libraries in place or provide alternate places for library updates that be used to be able to logically update the libraries at a given layer without disturbing the libraries in other layers. One advantage of using alternate locations for core and IDE library updates vs in place updates, is that the library manager could have the ability to revert back to the original library that was supplied in that layer. |
In my opinion, none of the proposed solutions will completely remove the problem.
Why are we doing it?
These libraries are usually declared as "architecture independent" since they are based on some other library like Wire or SPI which abstract the underlying hardware. Said so:
Possible solutions (always in my opinion):
|
Another even simpler solution is to never update the bundled libraries with the library manager. Just leave them as shipped with that version of the IDE and update with the next IDE release. |
solves arduino/ArduinoCore-samd#80, which was caused by USBHost library not having a corresponding .h, thus bypassing the findBestLibraryWithHeader check
e5a1602
to
d3b4fb1
Compare
Adding another bunch of compiled versions with the USBHost issue fixed |
Ok, I just took the time to read Bill's extremely long message. I agree completely, though I do wish it could be stated more concisely. As the maintainer of a 3rd party core, I believe architecture matching does not work. It's a beautiful ideal, much like Communism, which is simply incompatible with practical reality of human behavior. I publish one of the very widely uses 32 bit cores, and I must intentionally label it "avr" for compatibility. Numerous libraries that work perfectly fine are declared as "architecture=avr" or "architecture=avr,sam". If I create an architecture called "kinetis", my users suffer. Many libraries are unmaintained, or maintained by a busy individual, or maintained by small companies with limited resources who see no ROI in supporting anything over the 8 bit AVR. That is simply the reality of the Arduino world. It makes the architecture field pretty much worthless for any 3rd party boards/cores. Stated another way, consider the practical effect of the ever-expanding number of architectures. For architecture matching to work, ALL library authors (who don't use "architecture=*") must bear the maintenance cost for updating for the ever expanding list of architectures. It's a system that simply does not scale. |
@PaulStoffregen here is an attempt to boil it all down to something much more concise. One way to solve the problem, is to update the library manager to give each library in each priority location/layer the ability to update itself using its own repository. This completely solves the problem. Each library can update itself independently using its own repository. This is what allows the user, the core, and IDE to update their own version of the library and never break any other layers version of the library all while preserving the priority overrides. Another much simpler approach is what Paul just mentioned. @facchinm I also believe that many of these types of discussions drag on because the "problem" is not being fully defined and written down. So, again, because of different vantage points, different people will view the "problem" and what needs to be solved differently. In terms of the latest proposals: If a library only works with a given sub library within a particular core, then it should be declaring that it only works with that architecture. A library must properly declare its target architectures. In my view your "Said so" bullets are creating unnecessary restrictions and propagating the existing library manager problem of using a single library repository that always stomps on the users library area to do updates. I believe that any complete solution should to be generic and work for both core and bundled libraries as the big picture issues an needs for each of the layers is the same. The base problem here is that the library manager is too simplistic in the way it handles its updates. One way to handle all that is to look at what I proposed, which is to allow each individual library to have its own repository and update location rather than try to use a single repository for a library with a single update location. When libraries and library selection is based on fixed prioritized layers where each library in each layer can manage its own version of the library, the problem goes away and you gain the flexibility that any library in any layer can update itself without affecting any other version of the library in another layer. |
@PaulStoffregen This is an example where 3rd party s/w developers have to jump through hoops because the s/w that they have to integrate into or be compatible with cannot be easily updated or is somewhat "broken". There is no easy answer to this. While definitely related, I think it is a larger issue than fixing the update manager and builder better handle library updates. |
@bperrybap , As per my vantage point ( 😉 ), your view suffer from a problem: all libraries are created equal, and there is no way to differentiate two libraries beside their position in the filesystem This PR could solve the great majority of the problems which rise from the actual "dummy" update system. Of course making the library updater smarter would only bring additional benefits but it would also bring a lot more complexity (and compatibility problems with existing installations/making IDE regression tests with the same sketchbook impossible etc etc) |
@facchinm I very much disagree with your assessment of my view and proposal. I believe that priorities are easily handled by using library layers. I believe that the real issue that needs to be solved is how to do library updates. The current library manager with respect to updates takes a far too simplistic view:
This is unworkable and that is the problem we are seeing. No matter how the library priorities are arranged and what overriding rules are in put in place, I see no way to handle the above example through just priorities using a single installation location for library updates. The only way to fully solve the update issue is to provide the ability of each library in each layer to be updated individually. |
@ArduinoBot build this please |
Hi @bperrybap , sorry for the delay. |
@facchinm please consider documenting the changes proposed in this PR with an updated version of the include priorities list I created at arduino/Arduino#4064 (comment). This will make it more easy to understand and also illustrate how much complexity is being added. I think an ideal solution would add the minimum possible number of items to the list while fixing the issue in all cases. |
@facchinm - Does this solve any problem other than Arduino updating the IDE-bundled libs between official IDE releases? If the libs provided with a core only update when the entire core is updated, why can't the same be true for the IDE's bundled libs? The bundled libs are very mature and stable, and Arduino releases a new IDE a few times a year? Is Library Manager updating of the IDE's bundled libs really needed? My concern is the ever-increasing complexity of Arduino's build process, and the overly complicated design of most of the code added in the last year to 18 months. No single snowflake does much harm, but over time more and more can build up to an avalanche. |
@facchinm
Consider the case where you have IDE X.X.X installed. There is a bundled library update for a library ZZ (dumped in the users sketchbook). Then, a newer IDE X.X.Y is installed that has a newer version of library ZZ with additional bug fixes or expanded capabilities. My view is similar to Paul's in that from my perspective there really isn't any difference between a core and a bundled library update. I share Paul's concern about complexity of the build process. To me, at this point in time, the only viable option "to break less things possible", is to limit the library manager to only updating 3rd party libraries. (essentially, what Paul proposed about not supporting updating IDE bundled libraries) For the longer term, I think the library manager needs a re-design. In my view this is the direction that the library updates needs to go. |
@bperrybap your proposal is perfect, the table by @per1234 too, but we still miss the code for both Java IDE and builder and, above all, we still miss a complete series of regression tests for validating the new behaviour. |
With this change, does a library in sketchbook get trumped by a lib in core even if the lib in core also has Arduino has a fork of Firmata and pulls from the main Firmata repo before releasing an new IDE. My concern is that with this new rule (if I'm understanding it correctly) that if a user updates Firmata via the Library Manager it will always be trumped by the (likely older) version of Firmata in the Arduino core. I typically update Firmata more frequently than the Arduino IDE release cycle so that is my concern. |
Hi Jeff, |
Okay so the version of Firmata that ships with the Arduino IDE is considered "bundled" and therefore not affected by this update. SGTM then. |
As @PaulStoffregen suggested, Arduino.cc could simply opt to stop supplying bundled library updates until the library manager is properly fixed by disabling and removing all the arduino.cc bundled libraries from the library manager. It looks like this is a very simple temporary work around to simply bump the fixed priority order to make core libraries be the highest priority. So the new priority is now:
Which is what I said in my very first post. And so the problem of arduino.cc bundled lbrary update stomping on top of user libraries will still be a problem. This patch sounds like it will help in some cases but still break things in other cases. |
Moving core priority above sketchbook has been discussed a lot and it received very negative feedback. We can't drop bundled libraries because people need this feature (for example in schools installations). |
But from my understanding, the patch you are proposing is moving core priority above sketchbook. In order to get a core library to trump a user sketchbook library in any situation, you have to re-arrange the priorities so that core is above user sketchbook. I sounds like that is what this is being done in this patch. That said, this proposal is silent on what happens in some situation. And what about libraries that don't have the exact same name? I have great concerns about this for the broader picture and longer term. For school installations, it would seem that "update in place" would be the ideal solution for bundled updates. |
Fixes arduino/Arduino#4064
The rationale behind this patch:
Example with Ethernet library:
If a library gets updated upstream with the support for
xxx
architecture, thexxx
core should simply remove the modified "fork" from its repo